Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some error handling to OTP secret migration #30344

Merged
merged 1 commit into from
May 20, 2024

Conversation

ClearlyClaire
Copy link
Contributor

@ClearlyClaire ClearlyClaire commented May 17, 2024

Attempt to address #28325 (comment) by aborting with a more understandable error spelling out that decrypting OTP secrets failed and that it is most likely due to a past change to OTP_SECRET.

Also allows ignoring such failures through an environment variable.

== 20240307180905 MigrateDeviseTwoFactorSecrets: migrating ====================

ERROR: Unable to decrypt OTP secret for user @shanon0.

This is most likely because you have changed the value of OTP_SECRET at some point in
time after @shanon0 configured 2FA.

In this case, their OTP secret had already been lost with the change to OTP_SECRET, and
proceeding with this migration will not make the situation worse.

Please double-check that you have not accidentally changed OTP_SECRET just for this
migration, and re-run the migration with MIGRATION_IGNORE_INVALID_OTP_SECRET=true.

Migration aborted.

@ClearlyClaire ClearlyClaire force-pushed the fixes/invalid-otp-secret-error-handling branch from 446f7df to f9ad481 Compare May 17, 2024 14:39
Copy link
Contributor

@mjankowski mjankowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight fear that this message will be more confusing to people than if we just silently continued the migration when errors occurred, but I can't think of a more clear way to write it, given what we're trying to say.

I guess we also don't really have a way of knowing what portion of people will ever see this message, and of those how many actually have any users who have not abandoned their accounts but unknowingly have broken 2FA. Must be a small number.

All that said, I think this is a net improvement over the current scenario and it handles the error condition at least.

@ClearlyClaire ClearlyClaire added this pull request to the merge queue May 20, 2024
Merged via the queue into main with commit 2bcbeed May 20, 2024
51 checks passed
@ClearlyClaire ClearlyClaire deleted the fixes/invalid-otp-secret-error-handling branch May 20, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants